-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update AutoRest version to 47b422a4 #19959
Update AutoRest version to 47b422a4 #19959
Conversation
Add CustomEnityLookup skill and DocumentExtraction skill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some changes that violate guidelines.
sdk/search/Azure.Search.Documents/api/Azure.Search.Documents.netstandard2.0.cs
Show resolved
Hide resolved
sdk/search/Azure.Search.Documents/api/Azure.Search.Documents.netstandard2.0.cs
Show resolved
Hide resolved
/// <summary> Changes the default fuzzy edit distance value for this entity. It can be used to change the default value of all aliases fuzzyEditDistance values. </summary> | ||
public int? DefaultFuzzyEditDistance { get; set; } | ||
/// <summary> An array of complex objects that can be used to specify alternative spellings or synonyms to the root entity name. </summary> | ||
public IList<CustomEntityAlias> Aliases { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we've talked about it, but we haven't made this change yet, have we? I.e. collection properties shouldn't be settable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per internal discussion, these shouldn't be settable since they are collection properties. May be a bug in the generator. /cc @ShivangiReja @pakrym
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be the resolution?
An update to the generator followed by code regen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this property is nullable so we are forced to make it settable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Search we already had that issue and resolved it without making them settable. Previously we used the NullAsEmpty attribute property (or something like that), but I don't remember where we landed finally off hand. Pretty sure it wasn't settable collection properties. @Mohit-Chakraborty , I recommend looking at existing models with collection properties and see what we're doing.
cc: @tg-msft |
Closing this PR as we are updating to a new spec. |
Most significant changes included here are from -